fix(lock): implement Redlock single-instance pattern in LockManagerService - #537

@romanetar This 30 s lifetime makes TTL expiry a silent loss of mutual exclusion, and this call site is the only one of the four where the Redis lock is the sole guard of the invariant — the fix should be a DB pessimistic lock, matching the sibling tasks.

The other three sites are already backed by row locks — ReserveTicketsTask::run/undo hold getByIdsExclusiveLock/getByIdExclusiveLock on the ticket types and RedeemPromoCodesTask holds getByValueExclusiveLock on the promo code — so the DB serializes competing writers regardless of what Redis does. This callback instead reads the promo code via $this->summit->getPromoCodeByCode() with no lock, and the check-then-act sequence (hasPrePaidTicketsAssignedBy → getNextAvailableTicketPerType → addTicket) relies entirely on this Redis lock. If the callback ever outlives the 30 s TTL, a concurrent request acquires the lock and can hand the same prepaid ticket to two attendees. This failure mode is new to this PR: on main the key effectively never expired (TTL was a unix timestamp, ≈55 years). There is also a second window the Redis lock cannot cover even when it works: it is released when the callback returns, but the enclosing tx_service->transaction commits afterwards, so a competitor can read pre-commit state.

Suggested fix — same pattern as RedeemPromoCodesTask in this file: inject ISummitRegistrationPromoCodeRepository into AutoAssignPrePaidTicketTask and fetch the promo code with getByValueExclusiveLock($this->summit, $promo_code_val) inside the transaction, instead of the unlocked getPromoCodeByCode(). That method takes PESSIMISTIC_WRITE with HINT_REFRESH (DoctrineSummitRegistrationPromoCodeRepository.php:617), so the entity is re-hydrated from committed DB truth after the row lock is granted, and the lock is held through commit — closing both the TTL-expiry hole and the release-before-commit window in one move. The Redis lock can stay as a fail-fast layer, consistent with the sibling sites.

Independently, releaseLock() discarding the deleteIfValueMatches() === false signal still deserves the log + metric suggested in the CodeRabbit thread on LockManagerService.php:79 — a mismatch at release is the only runtime evidence that a lock expired mid-callback anywhere in the system.

Verified both points against the code and applied fixes for both (will push shortly).

DB pessimistic lock for AutoAssignPrePaidTicketTask: injected ISummitRegistrationPromoCodeRepository into the task (wired from SagaFactory::$promo_code_repository, which was already available there) and replaced the unlocked $this->summit->getPromoCodeByCode($promo_code_val) with $this->promo_code_repository->getByValueExclusiveLock($this->summit, $promo_code_val) — same pattern as ApplyPromoCodeTask. That row lock (PESSIMISTIC_WRITE + HINT_REFRESH) now covers the whole check-then-act sequence (hasPrePaidTicketsAssignedBy → getNextAvailableTicketPerType → addTicket) and is held through the enclosing transaction's commit, closing both the TTL-expiry hole and the release-before-commit window. The Redis lock stays in place as the fail-fast layer.

releaseLock() observability: it now checks the deleteIfValueMatches() result and, on a mismatch, logs a warning (lock name + token) and increments a lock_manager.release_mismatch counter via the existing ICacheService::incCounter() — no new metrics infra introduced. Added a regression test asserting the counter fires when release fails.

Ran tests/SummitOrderServiceTest.php against the real DB/Redis containers — no regressions (the 3 prepaid-ticket tests are pre-existing markTestSkipped('broken test.'), unrelated to this change, left as-is/out of scope for this PR).

@romanetar incCounter now fails on every invocation under the Redis client this app actually uses.

The options-array form set($key, 1, ['EX' => $ttl, 'NX' => true]) is phpredis API, but the app pins Predis (config/database.php:176, REDIS_CLIENT default predis), and Predis's SET command passes arguments through verbatim — StreamConnection::writeRequest serializes each argument with strlen(strval($argument)). With an array argument that's an "Array to string conversion": under Laravel's error handler it throws ErrorException; otherwise the wire command becomes SET key 1 "Array" and Redis replies ERR syntax error (Predis\Response\ServerException). Neither exception is caught by retryOnConnectionError, which only catches PredisConnectionException|\RedisException. Additionally, the default $ttl = 0 would produce EX 0, which Redis rejects even in the correct form.

addSingleValue a few lines below uses the correct Predis variadic form — mirroring it fixes both problems:

if ($ttl > 0) {
    if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1;
} else {
    if ($conn->set($counter_name, 1, 'NX') !== null) return 1;
}
return (int)$conn->incr($counter_name);

incCounter has no callers in the repo today, so reverting this hunk entirely is also reasonable — it's outside the lock scope of this PR.

Confirmed and fixed. Reproduced the exact failure against the real Predis-backed Redis container before the fix — Array to string conversion warning followed by Predis\Response\ServerException: ERR syntax error on every call, uncaught by retryOnConnectionError since it only catches PredisConnectionException|\RedisException, exactly as described.

Applied your suggested fix, mirroring addSingleValue's variadic form:

public function incCounter($counter_name, $ttl = 0)
{
    return $this->retryOnConnectionError(function ($conn) use ($counter_name, $ttl) {
        if ($ttl > 0) {
            if ($conn->set($counter_name, 1, 'EX', (int)$ttl, 'NX') !== null) return 1;
        } else {
            if ($conn->set($counter_name, 1, 'NX') !== null) return 1;
        }
        return (int)$conn->incr($counter_name);
    }, 0);
}

Verified against the real container: first call sets the key (returns 1), subsequent calls hit the NX failure path and fall through to incr (2, 3, ...), both with and without a TTL. No exceptions.

Kept incCounter rather than reverting the hunk, since it's now the target of the lock_manager.release_mismatch counter added in LockManagerService::releaseLock for the other thread on this PR — so it does have a caller as of this branch.

@romanetar This test never runs in CI: the push.yml suite matrix (.github/workflows/push.yml:57-68) is a fixed list of paths/filters with no entry for tests/Integration/, so the driver-compatibility and atomicity regressions this file exists to catch would pass silently. Redis is already provisioned in that workflow (supercharge/redis-github-action, line 148), so wiring it up is one line:

- { name: "Integration", filter: "tests/Integration/" }

While extending this class, please also add a case for deleteIfValueMatches — it's the Lua compare-and-delete the whole ownership guarantee rests on, and today it's only ever exercised against mocks. A broken script (or a driver change breaking the Predis-style eval($lua, 1, $key, $value) signature) would silently no-op every release, holding all locks to full TTL. Two assertions suffice: matching token deletes the key; non-matching token leaves it intact.

Both done.

CI wiring: added { name: "Integration", filter: "tests/Integration/" } to the integration-tests matrix in .github/workflows/push.yml, right after the Services entry. Redis is already provisioned there via supercharge/redis-github-action, so no other setup was needed. Verified this is the correct invocation form (bare directory path, matching the OAuth2Tests/EntityModelUnitTests/AuditUnitTests entries) — vendor/bin/phpunit --filter 'tests/Integration/' actually matches nothing and prints "No tests executed!" since --filter runs a regex against test/class names, not paths, so that form would have silently run zero tests in CI.

deleteIfValueMatches coverage: extended RedisCacheServiceAddSingleValueTest with two cases — matching token deletes the key, non-matching token leaves it intact (asserted both the return value and the key's survival/state). Ran all 4 tests in the file against the real Redis container: pass.

@romanetar The key fix is correct, but note it splits the lock namespace between fleets during a rolling deploy: old pods lock ticket_type.7promo_code.X.sell.lock while new pods lock ticket_type.7.promo_code.X.sell.lock, so the two fleets don't mutually exclude on this path for the duration of the rollout. Compounding it, old pods still run the unconditional-DEL-on-failed-acquire behavior this PR fixes, so they can delete new pods' token locks on any key. Since this call site is the one place where the Redis lock is the sole concurrency guard (no DB row lock — see my other comment), the deploy window is exactly where a race could materialize.

No code change needed — suggest a line in the release notes: deploy while prepaid-assignment traffic is idle, or drain old workers before starting the new fleet.

Good flag, and confirmed against main's LockManagerService — releaseLock() there is an unconditional delete($name) and lock()'s finally calls it even after acquireLock throws UnacquiredLockException, so an old pod that loses contention on any key will delete it out from under whoever holds it, no ownership check. Combined with the key-namespace split you describe (missing dot on old pods vs. present on new), the two fleets neither mutually exclude on this path nor leave each other's locks alone during the rollout — exactly the window you're calling out.

No code change from this thread, but worth noting: the DB pessimistic lock added for your other comment on this same call site (getByValueExclusiveLock on the promo code, PESSIMISTIC_WRITE + HINT_REFRESH, held through commit) closes the concrete failure mode here — old and new pods share the same MySQL row lock regardless of what happens to the Redis key naming or unconditional deletes on either side. So the double-assignment scenario this thread worried about is now covered even mid-rollout; Redis staying inconsistent across fleets during the deploy window would at most cause spurious contention/retries, not an actual double-booked prepaid ticket.

Will add the release-note line about draining old workers / deploying during idle prepaid-assignment traffic anyway, since the underlying old-pod unconditional-release bug can still stomp on other locks in the system during the transition, not just this one.